docs(README): note pull_request_target for fork-based flow #24
Conversation
|
And looks like it needs to be changed in your workflow :-) |
As noted here: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ With proposed doc PR in #24
|
@mikehardy Would you like to rebase this on the latest master so we can try if the token works correctly? |
|
Sure - I used the web UI for the patch workflow, let me clone local rebase and push |
|
boom! 👍 |
README.md
Outdated
| @@ -4,6 +4,8 @@ This is a [Github Action](https://github.com/features/actions) that ensures that | |||
|
|
|||
| This is helpful when you're using [semantic-release](https://github.com/semantic-release/semantic-release) with the Conventional Commits preset. When using the "Squash and merge" strategy, Github will suggest to use the PR title as the commit message. With this action you can validate that the PR title will lead to a correct commit message. | |||
|
|
|||
| Note that if you have a fork-based workflow you will want to use [`pull_request_target`](https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/) in place of `pull_request` so the API token is valid for status reporting. | |||
There was a problem hiding this comment.
Would it make sense that we just use pull_request_target in the snippet below or is there any drawback to this?
If so, I guess we could move this section to the bottom of the README and re-word it to something like:
Note that the usage of
pull_request_targetas the event trigger is necessary for a fork-based workflow so the API token is valid for status reporting.
What do you think?
There was a problem hiding this comment.
🤔 good question. I think pull_request_target is probably the best. We're not comparing code so it's okay the code used for the check is from the branch point (which is how pull_request_target protects secrets etc). It's just the title.
I can't think of any drawback to having pull_request_target be the default, but without it this fails on fork-based workflows.
So I think pull_request_target is likely the best default yes
Switch the actual default in the snippet instead of just explaining Then move explanation for those curious to the bottom
|
Okay @amannn I agree with both your suggestions and just implemented them. I was using the Web UI to do it quickly so now there are 3 commits where one would do but I assume you can squash :-). |
|
Perfect, thanks a lot for your help! |
Thinking about integrating this but did a deep-dive on the issues first, seems like one of the biggies is solved now but not noted yet since it's new info, here's a maybe-useful edit?